Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add autofill support to ios text input plugin #17493

Merged
merged 13 commits into from
Apr 16, 2020
Merged

Add autofill support to ios text input plugin #17493

merged 13 commits into from
Apr 16, 2020

Conversation

LongCatIsLooong
Copy link
Contributor

Framework PR: flutter/flutter#52126

Moved FlutterTextInputView's interface to the header file in order to expose it in unit tests.

@auto-assign auto-assign bot requested a review from gaaclarke April 3, 2020 02:51
@LongCatIsLooong LongCatIsLooong mentioned this pull request Apr 3, 2020
Merged
13 tasks
@ganttastic ganttastic mentioned this pull request Apr 5, 2020
Closed
gaaclarke
gaaclarke reviewed Apr 13, 2020
View reviewed changes
shell/platform/darwin/ios/framework/Source/FlutterEngine.mm
@@ -534,6 +534,11 @@ - (void)updateEditingClient:(int)client withState:(NSDictionary*)state {
arguments:@[ @(client), state ]];
}

- (void)updateEditingClient:(int)client withState:(NSDictionary*)state withTag:(NSString*)tag {
[_textInputChannel.get() invokeMethod:@"TextInputClient.updateEditingStateWithTag"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be safer if you used performSelector:withObject:withObject: https://developer.apple.com/documentation/objectivec/1418956-nsobject/1418667-performselector?language=objc

That will give you static analysis that the method you are invoking exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @"TextInputClient.updateEditingStateWithTag" does not represent an objective-c selector but a platform message (that will be received and handled by some dart code)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, you're right.

gaaclarke
gaaclarke reviewed Apr 13, 2020
View reviewed changes
shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.h Outdated
@@ -10,6 +10,9 @@
#include "flutter/shell/platform/darwin/common/framework/Headers/FlutterChannels.h"
#include "flutter/shell/platform/darwin/ios/framework/Source/FlutterTextInputDelegate.h"

#if FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DEBUG
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: This will be unnecessary after #17624 lands

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was talking about the "FLUTTER_EXPORT" not being needed once that lands. We used to have to export things we wanted to tests, now tests live with the code so it isn't necessary.

gaaclarke
gaaclarke reviewed Apr 13, 2020
View reviewed changes
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please date the PR with a more descriptive title.

gaaclarke
gaaclarke reviewed Apr 13, 2020
View reviewed changes
shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm Outdated
[super dealloc];
}

- (void)setTextInputClient:(int)client {
_textInputClient = client;
}

- (void)setAutofillId:(NSString*)autofillId {
_autofillId = [autofillId copy];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't release the old value. Instead of writing this you can just do@property(nonatomic, copy) NSString* autofillId to have one automatically generated. Put it in the .mm file to make it private.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review! I have to put the interface in the header file in order to be able to import that in a unit test (at least for now). Does adding autorelease after copy work here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh your pull request is already merged. I guess that means I can move the interface back to the .mm file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, autorelease can be used on any object.

gaaclarke
gaaclarke reviewed Apr 13, 2020
View reviewed changes
shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm Outdated
} else {
_activeView = _view;
NSAssert(clientUniqueId != nil, @"The client's unique id can't be null");
for (FlutterTextInputView* v in _inputViews)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

for (int i = 0; i < 10; i++)
  BlowTheHorn();                // AVOID.

http://google.github.io/styleguide/objcguide.html#conditionals

gaaclarke
gaaclarke requested changes Apr 13, 2020
View reviewed changes
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry i didn't put all my comments all together in one review, i'm done now =)

@LongCatIsLooong LongCatIsLooong changed the title Autofill ios Add autofill support to ios text input plugin Apr 13, 2020
gaaclarke
gaaclarke approved these changes Apr 14, 2020
View reviewed changes
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just threw on a change with my outstanding comments into a patch. LGTM now.

@LongCatIsLooong LongCatIsLooong merged commit 104df6b into flutter:master Apr 16, 2020
@LongCatIsLooong LongCatIsLooong deleted the autofill-ios branch April 16, 2020 09:48
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 16, 2020
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 16, 2020
@engine-flutter-autoroll
104df6b Add autofill support to ios text input plugin (flutter/engine…
012217b 
…#17493)
@xster xster mentioned this pull request Apr 18, 2020
Merged
xster added a commit to xster/engine that referenced this pull request Apr 22, 2020
@xster
review and add more functional change to match flutter#17493
636285d
mikkokaar pushed a commit to mikkokaar/engine that referenced this pull request Apr 24, 2020
@xster @mikkokaar
review and add more functional change to match flutter#17493
ba28fee
xster added a commit to xster/engine that referenced this pull request Apr 24, 2020
@xster
review and add more functional change to match flutter#17493
279d239
@jmagman jmagman mentioned this pull request May 25, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaaclarke gaaclarke gaaclarke approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LongCatIsLooong @gaaclarke @googlebot